Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remove pallet-history-seeding and all related stuff #3303

Merged
merged 3 commits into from
Dec 24, 2024

Conversation

dariolina
Copy link
Member

Removing history seeding from runtime as planned all along: we don't want to have the risk of uploading data for free.
However, this seems to break @teor2345 functionality to get object mapping of the data that was already seeded at genesis if the runtime call is gone. Keeping this draft until we can align with Teor on how to address that (if it's indeed an issue).

Code contributor checklist:

@teor2345
Copy link
Contributor

teor2345 commented Dec 10, 2024

Removing history seeding from runtime as planned all along: we don't want to have the risk of uploading data for free.
However, this seems to break @teor2345 functionality to get object mapping of the data that was already seeded at genesis if the runtime call is gone.

This is a tricky change, because we need to know the encoding of the call to extract the data from it. If we’d used remark for history seeding, then the encoding would be provided by another crate (which we’ll never remove).

But since we used a custom history seeding call, I think we’ll need to keep a seed_history call stub, which should always return an error (to prevent future calls). To host that stub, we’ll also need a history seeding crate stub. The benchmarks, tests, CheckHistorySeeder, seeder account, and other calls can be deleted.

I don’t know if the weights can be deleted, is it ok to set the weight of a call that always fails to zero? (Or an extremely large number?) Or will that cause trouble with past transaction validation, or future invalid transaction handling?

Another possibility is to delete the crate entirely, and use the binary encoding of the call. But I think we’d need to modify the parser to do that, because once we delete the history seeder, those calls will be parsed as unknown or invalid.

Maybe other people on the team can think of other alternatives?

@dariolina
Copy link
Member Author

@nazar-pc do you have an opinion here?

@nazar-pc
Copy link
Member

But since we used a custom history seeding call, I think we’ll need to keep a seed_history call stub, which should always return an error (to prevent future calls)

No, we do not. The mapping is determined by the runtime, specifically mapping for a particular block is dictated by the code in parent block's runtime and on the client side transactions are just opaque blobs. Runtime API that client calls will not change if we change the set of pallets, that is the whole point of the runtime API as such. So we can safely remove the pallet and mapping will continue working properly for both old and new blocks as long as runtime API doesn't change or client is updated to support both versions if we do have to change it at some point in the future.

@shamil-gadelshin
Copy link
Member

Is there a way to convert this feature to a devnet-only version? The seed-history-publisher seems to use this module and it is a convenient way to populate the network with data.

@nazar-pc
Copy link
Member

If your only goal is to populate the network with data, you can use system::remark extrinsic instead

@dariolina
Copy link
Member Author

@teor2345 can you take this over? From what I read above we only need to revert the change to object mappings in the first commit and test whether they still work as expected.

@teor2345
Copy link
Contributor

@teor2345 can you take this over? From what I read above we only need to revert the change to object mappings in the first commit and test whether they still work as expected.

My understanding is that this change is correct as is. Previously seeded history will be mapped correctly by the current version of the runtime.

Future history seeds won’t happen, so there’s no need for the next version of the runtime to support them. (And no practical way to test without deploying the current version of the runtime, then upgrading it to a version with this change.)

@teor2345 teor2345 marked this pull request as ready for review December 16, 2024 06:38
@teor2345 teor2345 self-assigned this Dec 16, 2024
@teor2345 teor2345 added the breaking-runtime This PR introduces breaking changes to the runtime label Dec 16, 2024
vedhavyas
vedhavyas previously approved these changes Dec 16, 2024
@dariolina
Copy link
Member Author

As discussed on Protocol sync, this needs runtime version increase before merge.

@vedhavyas
Copy link
Member

@teor2345 can you bump the runtime spec version and get this in ?

@teor2345

This comment was marked as resolved.

@teor2345 teor2345 added this pull request to the merge queue Dec 24, 2024
Merged via the queue into main with commit be615eb Dec 24, 2024
8 checks passed
@teor2345 teor2345 deleted the remove-history-seeding branch December 24, 2024 03:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-runtime This PR introduces breaking changes to the runtime
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants